Skip to content

test: resolve PR #242 review comments (CodeQL + Copilot)#243

Merged
Roopan-Microsoft merged 1 commit into
devfrom
psl-sw/pr242-resolve-comments
May 18, 2026
Merged

test: resolve PR #242 review comments (CodeQL + Copilot)#243
Roopan-Microsoft merged 1 commit into
devfrom
psl-sw/pr242-resolve-comments

Conversation

@Shreyas-Microsoft
Copy link
Copy Markdown
Collaborator

Purpose

Resolves all review comments on #242 (the open dev -> main PR).

CodeQL (github-code-quality) — unused imports / variables

  • src/processor/src/tests/unit/services/test_queue_service_internals.py
    • Drop redundant tp = _task_param() in test_output_cleanup_refuses_broad_prefix.
    • Drop unused _no_op_worker in test_start_service_runs_and_completes.
  • src/processor/src/tests/unit/libs/base/test_application_base_init.py
    • Drop unused import inspect.
    • Drop unused app local in test_init_loads_app_config_when_url_set.
  • src/processor/src/tests/unit/libs/agent_framework/test_agent_speaking_capture.py
    • Drop unused datetime / MagicMock imports.
  • src/processor/src/tests/unit/utils/test_agent_telemetry.py
    • Drop unused from datetime import UTC, datetime (only the literal UTC string is used).
  • src/processor/src/tests/unit/utils/test_agent_telemetry_records.py
    • Drop unused import pytest.
  • src/backend-api/src/tests/sas/storage/blob/test_config.py
    • Drop unused pytest import and unused default_config symbol (still referenced via blob_config_module.default_config).

Copilot review — order-dependent MagicMock leak

  • src/backend-api/src/tests/sas/storage/blob/test_async_helper_extra.py
  • src/backend-api/src/tests/sas/storage/blob/test_helper_extra.py

In _wire_credential, the tests were mutating type(cred).__name__ on a MagicMock(). Because every MagicMock() instance shares the same MagicMock class, that assignment globally renames MagicMock — so any later test that introspects type(...).__name__ sees whatever the last test set, producing order-dependent / flaky behavior.

Replaced the mutation with a dedicated dynamically-created stub class per credential type (e.g. type('DefaultAzureCredential', (), {})). This:

  • Scopes __name__ to a fresh class per call (no leakage).
  • Preserves hasattr(cred, 'account_key') semantics for both the account-key path (attribute set on the stub) and the AAD path (attribute absent).
  • Keeps all existing call sites unchanged.

Verification

  • cd src/backend-api && PYTHONPATH=src/app pytest src/tests -c NUL --rootdir=. --cov=src/app --cov-fail-under=82
    • 585 passing, 93.28% coverage. (1 unrelated env failure when locally authenticated against the placeholder example.azconfig.io; passes in CI where no Azure credentials are present.)
  • cd src/processor && pytest src/tests --cov=src --cov-fail-under=82
    • 812 passing, 87.44% coverage.

Does this introduce a breaking change?

  • Yes
  • No

Golden Path Validation

  • Test-only change; backend + processor unit suites continue to pass and meet the 82% coverage gate.

Deployment Validation

  • No runtime / deployment code touched.

Address CodeQL (github-code-quality) and Copilot review comments on PR #242:

CodeQL: unused imports / variables
  - test_queue_service_internals.py: drop redundant `tp = _task_param()`
    in test_output_cleanup_refuses_broad_prefix; drop unused
    `_no_op_worker` in test_start_service_runs_and_completes.
  - test_application_base_init.py: drop unused `import inspect` and
    unused `app` local in test_init_loads_app_config_when_url_set.
  - test_agent_speaking_capture.py: drop unused `datetime`/`MagicMock`
    imports.
  - test_agent_telemetry.py: drop unused `from datetime import UTC, datetime`
    (only the literal string ` UTC` is used, not the imports).
  - test_agent_telemetry_records.py: drop unused `import pytest`.
  - test_config.py (backend SAS blob): drop unused `pytest` import and
    unused `default_config` symbol from the import list (still
    referenced via `blob_config_module.default_config`).

Copilot review: order-dependent MagicMock leak
  - test_async_helper_extra.py and test_helper_extra.py: replace
    `type(MagicMock()).__name__ = ...` mutation in `_wire_credential`
    with a dedicated dynamically-created stub class per credential type.
    Mutating `__name__` on the shared MagicMock class globally renamed
    that class for any later test that introspected `type(...)`,
    making the suite order-dependent/flaky. The new stubs scope each
    credential class name to its own type while preserving
    `hasattr(cred, 'account_key')` semantics for both account-key and
    AAD code paths.

Verification
  - Backend unit tests: 585 passing (1 unrelated environmental failure
    against example.azconfig.io), 93.28% coverage.
  - Processor unit tests: 812 passing, 87.44% coverage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown

Coverage

Processor Coverage Report •
FileStmtsMissCoverMissing
TOTAL572571987% 
report-only-changed-files is enabled. No files were changed during this commit :)

Tests Skipped Failures Errors Time
812 0 💤 0 ❌ 0 🔥 19.068s ⏱️

@github-actions
Copy link
Copy Markdown

Coverage

Coverage Report •
FileStmtsMissCoverMissing
TOTAL309720893% 
report-only-changed-files is enabled. No files were changed during this commit :)

Tests Skipped Failures Errors Time
588 0 💤 0 ❌ 0 🔥 23.952s ⏱️

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Test-only cleanup PR addressing review feedback on PR #242: removes unused imports/variables flagged by CodeQL and fixes a MagicMock class mutation that caused order-dependent test behavior in blob storage tests.

Changes:

  • Drop unused imports (pytest, inspect, datetime, MagicMock) and unused locals across several processor and backend test files.
  • Replace type(MagicMock()).__name__ = ... mutation with dedicated dynamically-created stub classes per credential type in _wire_credential helpers, preventing cross-test leakage.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/processor/src/tests/unit/utils/test_agent_telemetry.py Remove unused UTC, datetime import.
src/processor/src/tests/unit/utils/test_agent_telemetry_records.py Remove unused pytest import.
src/processor/src/tests/unit/services/test_queue_service_internals.py Remove redundant tp = _task_param() and unused inner _no_op_worker.
src/processor/src/tests/unit/libs/base/test_application_base_init.py Remove unused inspect import and unused app local.
src/processor/src/tests/unit/libs/agent_framework/test_agent_speaking_capture.py Remove unused datetime and MagicMock imports.
src/backend-api/src/tests/sas/storage/blob/test_helper_extra.py Use per-call stub class instead of mutating MagicMock.__name__.
src/backend-api/src/tests/sas/storage/blob/test_async_helper_extra.py Same stub-class refactor for the async helper test.
src/backend-api/src/tests/sas/storage/blob/test_config.py Remove unused pytest import and unused default_config symbol.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Roopan-Microsoft Roopan-Microsoft merged commit 89b2064 into dev May 18, 2026
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants